Skip to content

feat(auth): use os keyring for secure credential storage#1148

Open
l2ysho wants to merge 30 commits into
masterfrom
1115-use-os-keyring-for-credential-storage
Open

feat(auth): use os keyring for secure credential storage#1148
l2ysho wants to merge 30 commits into
masterfrom
1115-use-os-keyring-for-credential-storage

Conversation

@l2ysho
Copy link
Copy Markdown
Contributor

@l2ysho l2ysho commented May 20, 2026

Store sensitive API tokens and proxy passwords in OS keyring for improved security, falling back to file.

Store sensitive API tokens and proxy passwords in OS keyring for improved security, falling back to file.
@l2ysho l2ysho linked an issue May 20, 2026 that may be closed by this pull request
@github-actions github-actions Bot added this to the 141st sprint - Tooling team milestone May 20, 2026
@github-actions github-actions Bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels May 20, 2026
@l2ysho l2ysho added t-dx Issues owned by the DX team. and removed t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels May 20, 2026
getApifyClientOptions became async but several test sites still passed
the returned Promise directly to `new ApifyClient(...)`, leaving the
client with undefined token/baseUrl.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the tested Temporary label used only programatically for some analytics. label May 20, 2026
l2ysho and others added 14 commits May 20, 2026 13:35
Persist the backend marker in auth.json on first login and honor it on
subsequent runs so we don't re-probe the OS keyring on every CLI
invocation. Also drop the write/delete side of the probe — getPassword
on a non-existent entry is enough to detect an unavailable backend and
avoids unnecessary Keychain access.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The OS keyring is machine-global, so the unique
__APIFY_INTERNAL_TEST_AUTH_PATH__ per test only isolates auth.json, not
the keyring. After one test logged in, the leaked token made later
tests see getToken() return a value with no matching username/id and
throw "Corrupted local user info". useAuthSetup already pins in-process
tests to the file backend; do the same for the dist subprocesses
runCli spawns.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously, clearSecrets() was a no-op when the active backend was
'file', so a user who logged in normally and later set
APIFY_DISABLE_KEYRING=1 before running logout would leave their token
in the OS keyring with no in-CLI way to remove it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…in message

When the user explicitly opts out via APIFY_DISABLE_KEYRING=1, calling
the keyring "unavailable" and telling them to set the var they already
set is misleading. Split the file-backend branch into two: env-disabled
vs probe failure.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… keyring

Scoping the keyring change down: stripping the entire proxy object from
the userInfo write in getLoggedClient also dropped proxy.groups, which
breaks the log_in_out API test that compares auth.json to the API user
response. Leave proxy in the file as it was before and exclude the
internal secretsBackend marker from the test comparison.
- Move `ensureApifyDirectory` from utils.ts to files.ts to break the
  credentials.ts <-> utils.ts circular import.
- Drop the synthetic write/read/delete probe at backend selection time
  and treat the first real keyring write as the probe. On failure,
  downgrade to the file backend and persist the marker so future runs
  skip the keyring entirely. Avoids a duplicate macOS Keychain prompt
  on first login.
- Reword the "Corrupted local user info" error to accurately describe
  the stale-keyring-without-metadata state.
The shared runId in the runs lifecycle test points at a hello-world
actor that finishes in well under a second. After the resurrect step,
the abort CLI's startup overhead (1–3s) meant the resurrected run had
typically already SUCCEEDED by the time abort fired its API call,
causing abort to print `Error: Run with ID "..." is already aborted.`
to stdout with exit code 0 — which then broke `JSON.parse` in the test.

Start a dedicated abort target run with a 30s `sleepMs` input so the
abort always wins the race. Teach the basic test actor to honor the
new `sleepMs` input.
@l2ysho l2ysho marked this pull request as ready for review May 28, 2026 15:26
@l2ysho l2ysho requested a review from vladfrangu as a code owner May 28, 2026 15:26
@l2ysho
Copy link
Copy Markdown
Contributor Author

l2ysho commented May 28, 2026

Lets do some first round of review while I am testing on different systems cc @DaveHanns @patrikbraborec @vladfrangu

l2ysho added 2 commits May 29, 2026 13:22
When the CLI runs from a Bun-compiled bundle (install-cli.sh path), the
@napi-rs/keyring native module isn't included in the binary, so login
always falls back to file storage. The new branch surfaces this to the
user with an actionable hint — install via npm for keyring storage —
instead of the generic "OS keyring unavailable" message, which suggests
a fix (APIFY_DISABLE_KEYRING=1) that doesn't apply here.

Tracking issue for actually shipping the native binary inside the
bundle: see BUNDLE_KEYRING_ISSUE.md in the repo root.
Copy link
Copy Markdown
Contributor

@DaveHanns DaveHanns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few nits and suggestion, some questions.
Looks great otherwise 🚀

Comment thread src/lib/credentials.ts
import { ensureApifyDirectory } from './files.js';
import { cliDebugPrint } from './utils/cliDebugPrint.js';

const KEYRING_SERVICE = 'com.apify.cli';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: each macOS Keychain item carries an ACL (Access Control List) of binaries allowed to read it, keyed by the creating binary's code signature, not to the service name (com.apify.cli). This CLI is commonly run as different binaries (a globally installed apify, npx apify-cli, a project-local node_modules/.bin/apify), so a token written by one may not be readable by another. macOS prompts (AFAIK) for access in an interactive shell, or fails outright in a headless/CI session. The auth.json had no such gate.

Not a blocker (and a non-issue if @napi-rs/keyring stores with a permissive ACL), but worth validating on macOS before merge.
The failure has to be graceful (clear error + relogin prompt), especially when headless where there's no dialog to approve.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be a time where we want to start signing our macOS bundles? https://bun.com/docs/bundler/executables#code-signing-on-macos bun supports it, but we would need an apple certificate

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, sounds like the solution.
I will create a ticket and we can tackle that additionally later.

Comment thread src/lib/credentials.ts Outdated
Comment thread src/lib/credentials.ts Outdated
Comment thread src/lib/credentials.ts Outdated
Comment thread src/lib/credentials.ts Outdated
Comment thread src/lib/utils.ts Outdated
Comment thread src/lib/utils.ts Outdated
Comment thread src/lib/credentials.ts
migrationPromise = (async () => {
try {
const file = readAuthFile();
if (file.secretsBackend) return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:
with any failure to get / set keyring secret, we fall back to the file backend and this early return makes it permanent.

The failure might be temporary service outage and yet, user never recovers from this up until their logout.

Maybe we should attempt to migrate more often, for instance, when the getBackend is first called (backendPromise is undefined).

If we go this route, then we also have to deal with getBackend's if (marker === 'file') return 'file' as that locks in the file as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true but I think the percentage of users with this problem will be pretty low (temporary service outage scenario). I believe for someone with some exotic Unix distro this can happen, but 95% of all users should be fine. But if you have a concerns about it we can create a followup issue (this may be a big chunk of work to just add it here)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. I'll create a ticket to revisit this later and only if we get reports.

Comment thread src/lib/credentials.ts Outdated
Comment thread src/lib/utils.ts Outdated
l2ysho and others added 12 commits June 2, 2026 22:25
Co-authored-by: David Hanuš <dh.david.hanus@gmail.com>
…al-storage' into 1115-use-os-keyring-for-credential-storage
The function only deletes keyring entries; auth.json cleanup is the caller's
responsibility (logout rimrafs the whole file). Renaming makes the scope
honest and avoids misleading future callers.
…rameter

Avoids the param-reassignment smell in getLoggedClient and
getApifyClientOptions. oxlint's no-param-reassign rule is currently
disabled in config; enabling it broadly is left for a follow-up.
Factor out hasUserMetadata so the stale-credentials branch reads
directly. Behaviorally identical to the previous two-stage check.
@l2ysho l2ysho requested a review from DaveHanns June 2, 2026 22:51
Comment thread src/lib/credentials.ts
Comment on lines +241 to +246
writeAuthFile(file);
return;
}

delete file.token;
delete file.proxy;
Copy link
Copy Markdown
Contributor

@DaveHanns DaveHanns Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: same problem as above. We don't delete only the password here, but also any sibling properties in proxy.

Not tested, but maybe following would work? If not, conditionally deleting should.

Suggested change
writeAuthFile(file);
return;
}
delete file.token;
delete file.proxy;
writeAuthFile(file);
return;
}
delete file.token;
delete file.proxy?.password;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-dx Issues owned by the DX team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use OS keyring for credential storage

4 participants